Recreate: Migrate InitialListValueSelectorModal to dynamic @react-navigation route (with regression fixes)#94014
Conversation
…egression fixes Re-applies the migration of InitialListValueSelectorModal to a @react-navigation dynamic route (originally #91627) and adds fixes for the regressions it introduced: - Issue 93138: redirect back to the Add field page when the dynamic Initial value page is opened via refresh/deeplink without available list values, instead of rendering an empty picker. - Issue 93141: drop includePaddingTop={false} so the page no longer overlaps the device status bar on native (the prop was only safe inside the old RIGHT_DOCKED modal). - Issue 93186: sync the draft initial value back into the form so the validation error clears after a value is selected on the dynamic route. Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…icator Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Fixed the failing Root cause: Fix: Passed <FullScreenLoadingIndicator
reasonAttributes={{
context: "DynamicReportFieldsInitialListValuePage",
isLoadingFormDraft: !!isLoadingFormDraft,
}}
/>Verified locally with The other failing check, |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-06-21.at.12.03.39.AM.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-20.at.11.47.44.PM.mov |
|
@MelvinBot #93186 is still not fixed. |
…ute (issue 93186) Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
Good catch — the previous attempt at the Why it was still broken: the menu item only synced the draft value into the form when Fix: detect the draft change via a ref instead of comparing against the (already-synced) Local checks passed: prettier, lint (clean on the changed file), and |
|
@MelvinBot Mark it as ready for review |
melvin-bot
Bot
requested review from
joekaufmanexpensify and
thelullabyy
and removed request for
a team
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 I checked the CI status — all checks are now passing, so no fix is needed. The only The |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| onBackButtonPress={() => Navigation.goBack(backPath)} | ||
| /> | ||
| {isLoadingFormDraft || shouldRedirectToCreatePage ? ( | ||
| <FullScreenLoadingIndicator reasonAttributes={{context: 'DynamicReportFieldsInitialListValuePage', isLoadingFormDraft: !!isLoadingFormDraft}} /> |
There was a problem hiding this comment.
❌ UI-1 (docs)
FullScreenLoadingIndicator is rendered inside the same <ScreenWrapper> tree as the <HeaderWithBackButton> (lines 74-77). Because navigation (the back button) is visible alongside the loader, the user already has an escape route if loading hangs — so a full-screen loader is the wrong indicator here. This matches the UI-1 "Incorrect" example of FullscreenLoadingIndicator and HeaderWithBackButton living under the same JSX tree.
Use an ActivityIndicator centered in the content area instead, keeping the header rendered during loading:
{isLoadingFormDraft || shouldRedirectToCreatePage ? (
<View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter]}>
<ActivityIndicator size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} />
</View>
) : (
// ...
)}Reviewed at: 8ac54ef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done — pushed in df3bbec.
Per UI-1, since the HeaderWithBackButton stays rendered during loading (the user always has a back-button escape route), the full-screen loader was the wrong indicator. Swapped it for a centered ActivityIndicator, matching the sibling migrated page DynamicNewReportWorkspaceSelectionPage.tsx:269-275:
<View style={[styles.flex1, styles.fullScreenLoading]}>
<ActivityIndicator
size="large"
reasonAttributes={{context: 'DynamicReportFieldsInitialListValuePage', isLoadingFormDraft: !!isLoadingFormDraft}}
/>
</View>Local checks passed: prettier, lint (clean on the changed file), typecheck-tsgo, and React Compiler compliance.
| // required error lingers. We can't detect the change by comparing the draft against `value` (they're already in | ||
| // sync by the time this runs), so we track the previous draft value and push it through onInputChange whenever it | ||
| // changes. onInputChange triggers the form's onValidate, which clears the stale error. | ||
| const previousDraftInitialValue = useRef(draftInitialValue); |
There was a problem hiding this comment.
❌ PERF-10 (docs)
This useEffect exists solely to push a value up to the parent form via the onInputChange callback whenever the Onyx draft's initialValue changes. Using an effect to mirror state into a parent callback is the parent-communication anti-pattern PERF-10 targets — it adds an extra render pass, requires the manual useRef previous-value bookkeeping, and makes the data flow indirect.
Prefer having the parent (the create page / FormProvider) read the draft initialValue from Onyx directly and feed it into the form value, so the value flows down as a prop rather than being echoed back up from the child through an effect. That removes the need for this effect and the previousDraftInitialValue ref entirely.
Reviewed at: 8ac54ef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Doesn't seem to be correct, we need to call onInputChange to clear the stale required error
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@ShridharGoel Can you fix the author checklist and address all AI comments please
|
@MelvinBot Paste the below checklist in the description instead of the current checklist: Checklist
|
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
|
@aimane-chnaif @mountiny One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Re-checked after the latest CI run — all checks are passing, so there's nothing to fix. The only If you're seeing a red ❌ on the PR, it's that superseded |
|
The bot is not allowing us to fill in the author checklist, I will merge now, not emergency |
|
@MelvinBot Check all the checkboxes in the author checklist |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
3 participants
Explanation of Change
This recreates PR #91627 (which was merged and then reverted) and additionally fixes the three regressions it introduced.
Original migration (recreated):
Migrates
InitialListValueSelectorModalfrom areact-native-modal-backed<Modal>to a@react-navigationdynamic route screen, as part of the parent migration effort (#53493).WORKSPACE_REPORT_FIELDS_INITIAL_LIST_VALUEwith path suffixinitial-list-valueandREPORT_FIELDS_CREATEas its entry screenDynamicReportFieldsInitialListValuePagethat reads from / writes to theWORKSPACE_REPORT_FIELDS_FORMdraft via Onyx, preserving the existing toggle behavior (re-selecting clears the value)useDynamicBackPathfor correct back navigation including deeplink scenariosInitialListValueSelector/index.tsxto a stateless menu item that navigates to the dynamic route viacreateDynamicRouteInitialListValueSelectorModal.tsxRegression fixes added on top:
includePaddingTop={false}from the old modal. That was only safe inside the previousRIGHT_DOCKED<Modal>; on a standalone native screen it removes the top safe-area padding and overlaps the status bar. Removed the prop so the default top padding applies.onInputChange, so theFormProvidervalidation error was never cleared. The menu item now syncs the draft initial value back into the form, which updates the form value and clears the existing error.Fixed Issues
$ #90472
$ #93138
$ #93141
$ #93186
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screenshots/Videosundefined